Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Next version of the containers library #104

Merged
merged 21 commits into from
Mar 21, 2018
Merged

Next version of the containers library #104

merged 21 commits into from
Mar 21, 2018

Conversation

Hackerpilot
Copy link
Contributor

Goals:

  • More consistency in method names and argument order
  • More opportunities to use the containers without accidentally using the GC
  • Slightly better performance
  • Ability to use more containers as output ranges

@Hackerpilot Hackerpilot added this to the v0.8.0 milestone Mar 16, 2018
@Hackerpilot Hackerpilot changed the title WIP: Next version of the containers library Next version of the containers library Mar 19, 2018
@Hackerpilot
Copy link
Contributor Author

  • Fixed a few previously-undiscovered bugs.
  • Added documentation generation script.
  • Generated documentation will be served out of the docs/ folder in the master branch once this is merged.
  • Lots of documentation improvements and fixes.
  • Added a better string hashing function.
  • Improved space efficiency of the hash map.

@skl131313
Copy link

Is it a good idea to add the docs to the git repo? It looks like it is adding a ton of bloat, every function has its' own html file.

@wilzbach
Copy link
Member

Yes, especially if we already use Travis to auto-generate the documentation and push it to the gh-pages branch: https://dlang-community.github.io/containers/index.html

@Hackerpilot
Copy link
Contributor Author

I either didn't know or forgot that there was a CI job set up to keep the gh-pages branch up-to-date.

@Hackerpilot
Copy link
Contributor Author

Fixes #63.

@Hackerpilot
Copy link
Contributor Author

A version of the library before these changes is tagged as https://github.com/dlang-community/containers/releases/tag/v0.7.0.

@@ -110,6 +124,18 @@ version (D_InlineAsm_X86_64) struct SimdSet(T, Allocator = Mallocator)
return true;
}

/// ditto
bool opOpAssign(string op)(T item) if (op == "~")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation.

* Returns: the least recently inserted item.
*/
auto back(this This)() inout @property
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

front has a contract but this doesn't.

/**
* Returns: the value associated with the given key, or the given `defaultValue`.
*/
auto get(this This)(K key, lazy V defaultValue) inout @trusted
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key should probably be const here.

* key = the key to look up
* value = the default value
*/
auto getOrAdd(this This)(K key, lazy V value) @safe
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

/**
* Returns: true if the mapping contains the given key
*/
bool containsKey(K key) inout pure nothrow @nogc @safe
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

@@ -273,7 +294,7 @@ struct UnrolledList(T, Allocator = Mallocator,
import containers.internal.backwards : bsf;
import std.string: format;
size_t index = bsf(_front.registry);
assert (index < nodeCapacity, format("%d", index));
//assert (index < nodeCapacity, format("%d", index));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete or move to an in contract.

*
* Throws: RangeError if the key does not exist.
*/
auto opIndex(this This)(K key) inout
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const K

*/
bool insert(T value)
size_t insert(T value, bool overwrite = false) @safe
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should other containers also have overloads of insert that take ranges and return size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this off until later: #105

@@ -133,74 +169,113 @@ struct TTree(T, Allocator = Mallocator, bool allowDuplicates = false,
*/
bool remove(T value, void delegate(T) cleanup = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figure out what should be done here when duplicates are enabled. This should probably return size_t like insert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Arbitrarily decided that remove would only remove a single element at a time
  • documented this behavior
  • added a test

static if (is(typeof(less) == string ))
{

static if (less == "a < b" && isPointer!T
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document why this terrible, horrible hack is needed.

@Hackerpilot Hackerpilot merged commit 1b3885c into master Mar 21, 2018
@Hackerpilot Hackerpilot deleted the next branch March 21, 2018 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants